Skip to content

build: do not include shared gtest in node link path - #66626

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:gtest-test-only
Oct 11, 2026
Merged

build: do not include shared gtest in node link path#66626
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:gtest-test-only

Conversation

@aduh95

@aduh95 aduh95 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

gtest symbols are only used by cctest, but atm ./configure --shared-gtest results in -lgtest being added for all compilation, which results in readelf -d bin/node listing NEEDED libgtest.so. This PR introduces a separate node_shared_gtest_libraries list that is only provided to cctest recipe

@nodejs-github-bot

nodejs-github-bot commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. needs-ci PRs that need a full CI run. labels Oct 9, 2026
@aduh95

aduh95 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

@nodejs/quic looks like we're broken on main, likely due to the V8 upgrade:

../src/quic/application.cc:159:12: error: no member named 'SetPrototypeV2' in 'v8::Object'; did you mean 'SetPrototype'?
  159 |   if (obj->SetPrototypeV2(env->context(), Null(env->isolate())).IsNothing()) {
      |            ^~~~~~~~~~~~~~
      |            SetPrototype
../deps/v8/include/v8-object.h:484:37: note: 'SetPrototype' declared here
  484 |   V8_WARN_UNUSED_RESULT Maybe<bool> SetPrototype(Local<Context> context,
      |                                     ^
1 error generated.
make[2]: *** [node_base.target.mk:605: /home/runner/work/node/node/node/out/Release/obj.target/node_base/src/quic/application.o] Error 1
make[2]: *** Waiting for unfinished jobs....

@pimterry

pimterry commented Oct 9, 2026

Copy link
Copy Markdown
Member

Yep, there's a pending fix in #66603

@aduh95
build: do not include shared gtest in node link path
1853b55 
Signed-off-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@nodejs-github-bot

nodejs-github-bot commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/78297/

@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x labels Oct 11, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 3b639be into nodejs:main Oct 11, 2026
77 checks passed
@nodejs-github-bot

nodejs-github-bot commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

Landed in 3b639be

@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 11, 2026
@aduh95
aduh95 deleted the gtest-test-only branch October 11, 2026 23:25
@aduh95 aduh95 mentioned this pull request Oct 12, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UlisesGascon UlisesGascon UlisesGascon approved these changes
@richardlau richardlau richardlau approved these changes
+1 more reviewer
@bjohansebas bjohansebas bjohansebas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aduh95 @nodejs-github-bot @pimterry @UlisesGascon @richardlau @bjohansebas